Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Share dialog files sidebar #18185

Merged
merged 59 commits into from
Sep 16, 2015
Merged

Share dialog files sidebar #18185

merged 59 commits into from
Sep 16, 2015

Conversation

blizzz
Copy link
Contributor

@blizzz blizzz commented Aug 10, 2015

supposed to implement #17665

obsoletes #18078

  • TODO: modularize share dialog into reusable view and item model
  • TODO: ensure backwards compatibility with showDropDown() not needed
  • TODO: embed share dialog in share tab
  • TODO: write unit tests
  • REQUIRES: Introduce microevent library #9227

currently this is based on files-rightsidebar

cc @PVince81

@blizzz blizzz added this to the 8.2-current milestone Aug 10, 2015
@@ -10,7 +10,7 @@

(function() {
var TEMPLATE =
'<div>Owner: {{owner}}';
'<div><span>Owner: {{owner}}</span>';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blizzz blizzz mentioned this pull request Aug 10, 2015
5 tasks
@DeepDiver1975
Copy link
Member

So this code piece continues to use core/ajax/share.php up to now - right?

With moving this to the OCS Sharing API we have an issue as discussed in #17143 (comment)

Should we build a files only share side bar within files_sharing and use files_sharing only?
Or move ocs sharing api to core?

@DeepDiver1975
Copy link
Member

@jancborchardt @PVince81 @blizzz did we respect the share expiry for internal shares in the ui?
#15459 (comment)

No need to implement this right away - but we need to respect this in the UI design - THX

@PVince81
Copy link
Contributor

Should we build a files only share side bar within files_sharing and use files_sharing only?

@DeepDiver1975 this is the kind of decision that @schiesbn wanted to have with @rullzer at the conf.

I personally would vote for having a files-only share dialog for now and extend it later to make it generic, not the other way around. But I think @blizzz was also for having a generic sharing mechanism.

@DeepDiver1975
Copy link
Member

but talking about this at the conf is a bit late? We are building this now.

furthermore - if I remember correctly - we wanted to have share.js untouched - right?

Well - maybe I did miss some parts of the conversation 😢

@blizzz
Copy link
Contributor Author

blizzz commented Aug 11, 2015

I like to avoid to maintain two share dialogs and maintain and touch both of them any time we need to fix or extend it. Also, for know I just want to complete this work and have the share dialog integrated in sidebar for files. I prefer new features in a later pull request for not letting this one grow too big and consume too much time.

@DeepDiver1975
Copy link
Member

I like to avoid to maintain two share dialogs and maintain and touch both of them any time we need to fix or extend it.

well - from my pov share.js has to simply die.
Any app with the intention of implementing sharing can us core's sharing functionalities but the js side is app specific.

At least this is what we have been talking about @schiesbn @PVince81 @karlitschek

Don't get me wrong I'm not saying we need to address this now ... but we better keep this in mind

@blizzz
Copy link
Contributor Author

blizzz commented Aug 11, 2015

it will not receive improvements, but will be thinned out.

@DeepDiver1975
Copy link
Member

@DeepDiver1975 this is the kind of decision that @schiesbn wanted to have with @rullzer at the conf.

ok - agreed - please make sure I'm with you when you talk 😉

@jancborchardt
Copy link
Member

@blizzz let me know if there’s anything to review interface-wise. The dialog itself doesn’t change in this iteration, it just moves to the sidebar, right?

@DeepDiver1975
Copy link
Member

The dialog itself doesn’t change in this iteration, it just moves to the sidebar, right?

it is being reimplemented

@blizzz
Copy link
Contributor Author

blizzz commented Aug 11, 2015

Which means it stays visually the same, yes.

*
* @param itemType
* @param itemSource
* @param callback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to document the callback vs no callback sync behavior

@jancborchardt
Copy link
Member

This should also be rebased to reflect the actions in the dropdown change in master.

@PVince81
Copy link
Contributor

also note: the DetailsView and subviews are now backbone views.

@blizzz
Copy link
Contributor Author

blizzz commented Aug 18, 2015

rebased

@ghost
Copy link

ghost commented Aug 18, 2015

💣 Test FAILed. 💣
nooo432

var view = this;

//FIXME: specific to reshares stuff
this.model.on('change', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a bit of luck "change:reshares" could work 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, should be safe, thanks for the hint

@PVince81
Copy link
Contributor

Force-pushed. Let's hope CI will be kind this time.

@DeepDiver1975
Copy link
Member

jsunit and phpunit on sqlite did succeed -> merge

DeepDiver1975 added a commit that referenced this pull request Sep 16, 2015
@DeepDiver1975 DeepDiver1975 merged commit afc7eea into master Sep 16, 2015
@DeepDiver1975 DeepDiver1975 deleted the share-dialog-files-sidebar branch September 16, 2015 07:40
@jancborchardt
Copy link
Member

Wow. WOW!

Finally. :D great work @blizzz @PVince81

@jospoortvliet
Copy link

Congratulations everybody ;-)

This is awesome stuff.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.